Fixes to reflection cache#381
Conversation
ccummingsNV
left a comment
ccummingsNV
left a comment
There was a problem hiding this comment.
This one does make me a bit nervous, as I went through a lot of combinations of working out how to cache these things. If you're confident it's the right fix though, go for it.
|
If there are sanity checks you would want to run, or more extensive tests you can think of that would make you feel less nervous, I'm happy to add them. |
|
@tunabrain Is this still being worked on? |
|
@tunabrain , can you resolve the merge conflict? |
|
Merge conflict resolved. |
|
@ccummingsNV , can you review the change? |
📝 WalkthroughWalkthrough
ChangesFunction Caching Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
slangpy/reflection/reflectiontypes.py (1)
1603-1614:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReflection-only
SlangFunctions stop participating in hot reload.After this change,
_get_or_create_function()only records wrappers in_functions_by_reflection, buton_hot_reload()rebuilds state exclusively from_functions_by_name(Lines 1424-1445). Any function created throughfind_function(...)orspecialize_with_arg_types(...)now keeps a staleFunctionReflectionacross reload because there is no reload path for it anymore. The new overload test already creates one of these reflection-only entries on Line 110.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slangpy/reflection/reflectiontypes.py` around lines 1603 - 1614, The bug: functions created only via reflection are stored in _functions_by_reflection by _get_or_create_function but on_hot_reload rebuilds state solely from _functions_by_name, so reflection-only SlangFunction instances (created by find_function or specialize_with_arg_types using FunctionReflection) are not re-registered on reload and become stale; to fix, ensure reflection-created functions are also registered into the name-based registry during creation (or add a reload path that rebuilds from _functions_by_reflection). Concretely, update _get_or_create_function to, after calling _reflect_function(refl, this, full_name) and before returning, also insert the returned SlangFunction into _functions_by_name (using its unique name key as used by on_hot_reload) or extend on_hot_reload to iterate _functions_by_reflection entries and rebuild those functions; reference: _get_or_create_function, _functions_by_reflection, _functions_by_name, on_hot_reload, find_function, specialize_with_arg_types, FunctionReflection, SlangFunction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@slangpy/reflection/reflectiontypes.py`:
- Around line 1603-1614: The bug: functions created only via reflection are
stored in _functions_by_reflection by _get_or_create_function but on_hot_reload
rebuilds state solely from _functions_by_name, so reflection-only SlangFunction
instances (created by find_function or specialize_with_arg_types using
FunctionReflection) are not re-registered on reload and become stale; to fix,
ensure reflection-created functions are also registered into the name-based
registry during creation (or add a reload path that rebuilds from
_functions_by_reflection). Concretely, update _get_or_create_function to, after
calling _reflect_function(refl, this, full_name) and before returning, also
insert the returned SlangFunction into _functions_by_name (using its unique name
key as used by on_hot_reload) or extend on_hot_reload to iterate
_functions_by_reflection entries and rebuild those functions; reference:
_get_or_create_function, _functions_by_reflection, _functions_by_name,
on_hot_reload, find_function, specialize_with_arg_types, FunctionReflection,
SlangFunction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a671a61f-d2f0-4a06-9645-921c3c91fc17
📒 Files selected for processing (2)
slangpy/reflection/reflectiontypes.pyslangpy/tests/slangpy_tests/test_reflection2.py
4 participants
This is a (very simple!) fix to a reflection caching bug that causes a range of convoluted bugs and weird behavior.
Anytime we look up a function, whether by name or by reflection, we construct a new cache key representing the function name and save the reflection for future lookups and hot reloads.
This causes a few problems:
int foo<int X>() { return X; }, we can look up the specialized function"foo<5>". SlangPy checks the cache for prior lookups of"foo<5>", but saves it in the cache under the name"foo". This means lookups for specialized generics never hit the cache.Even weirder: Calling
foo()directly is not allowed, sinceXcan't be inferred. But if you called or looked up"foo<5>"at some earlier point, callingfoo()now suddenly succeeds, because the cache returns the functionfoo<5>instead when you look up"foo".void foo(int x) {} void foo(int2 x) {}, reflecting"foo"returns an overloaded reflection object. As a result of how the reflection API handles these, SlangPy instead caches this lookup underNone. OTOH, when slangpy specializes the function to get a specific overload (e.g.foo(int)), this one is cached under"foo". From then on, looking up the function"foo"by name returns that specific specialization, not the overloaded function, and trying to call e.g.foo(int2)after that could fail. It's surprising this hasn't caused issues before - our savior here is that theModulekeeps a separate_attr_cacheand avoids hitting the (incorrect) reflection cache most of the time.Nonefunction name.The fix is very simple, and moves the by-name-caching into the functions that look up by name, and caches under the exact name used to query the reflection API.
Summary by CodeRabbit
Refactor
Tests